Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cfn-include): allow dynamic mappings to be used in Fn::FindInMap #13428

Merged
merged 2 commits into from
Mar 6, 2021

Conversation

skinny85
Copy link
Contributor

@skinny85 skinny85 commented Mar 5, 2021

The template parsing logic in cloudformation-include always searched for the Mapping
in the template based on the first argument passed to Fn::FindInMap.
However, that doesn't work if that first argument is a dynamic expression,
like { Ref: Param }.

Check for that case explicitly, and don't search for the Mapping
if the first argument to Fn::FindInMap is a dynamic expression.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

The template parsing logic in cloudformation-include always searched for the Mapping
in the templatebased on the first argument passed to Fn::FindInMap.
However, that doesn't work if that first argument is a dynamic expression,
like `{ Ref: Param }`.

Check for that case explicitly, and don't search for the Mapping
if the first argument to Fn::FindInMap is a dynamic expression.
@skinny85 skinny85 self-assigned this Mar 5, 2021
@gitpod-io
Copy link

gitpod-io bot commented Mar 5, 2021

@github-actions github-actions bot added the @aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package label Mar 5, 2021
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 5, 2021
mappingName = value[0];
} else {
const mapping = this.finder.findMapping(value[0]);
if (!mapping) {
Copy link
Contributor

@NetaNir NetaNir Mar 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error seems at odds with allowing unresolved values. Is this error preventing something from breaking down the road? If so, will it break if this value is unresolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this error is to prevent situations where the argument to Fn::FindInMap is a static string (so something like { 'Fn::FinInMap': ['MappingName', 'Lvl1', 'Lvl2'] }), but a Mapping with that name could not be found in the template (so, continuing the above example, MappingName would not be in the template).

In case the value is dynamic, like in the test for this PR, we can't prevent this mistake from happening. We also can't handle things like re-naming the Mapping from CDK code correctly with dynamic mappings. But I think that's fine - we do the validation when we can, and when we can't, we just trust the template author they modeled things correctly.

@mergify
Copy link
Contributor

mergify bot commented Mar 6, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 4024ea7
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Mar 6, 2021

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 623675d into aws:master Mar 6, 2021
@skinny85 skinny85 deleted the fix/cfn-include-dynamic-findinmap branch March 6, 2021 01:57
cornerwings pushed a commit to cornerwings/aws-cdk that referenced this pull request Mar 8, 2021
…ws#13428)

The template parsing logic in cloudformation-include always searched for the Mapping
in the template based on the first argument passed to Fn::FindInMap.
However, that doesn't work if that first argument is a dynamic expression,
like `{ Ref: Param }`.

Check for that case explicitly, and don't search for the Mapping
if the first argument to Fn::FindInMap is a dynamic expression.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/cloudformation-include Issues related to the "CFN include v.20" package contribution/core This is a PR that came from AWS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants